Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Fix collapse animation #4493

Closed
wants to merge 2 commits into from
Closed

Fix collapse animation #4493

wants to merge 2 commits into from

Conversation

inukshuk
Copy link
Contributor

This is a tricky one. If you look at TWBS js, you will notice that the class in is added after the expand transition is complete and removed as the collapse transition starts This is particularly confusing because it works differently than .fade.in for example and it differs slightly from our current behaviour of adding the in class using $animate which adds the class immediately at the start of the transition.

This discrepancy is not visible in the collapse example on the homepage which opens and closes a well, but it is visible, for example when collapsing a navbar (mobile version) -- a very common use case in TWBS version 3. @flachware adjusted the Plunker example accordingly, and, when you open/close the menu, you will see a scrollbar during the expand animation (depending on your browser/OS handling of scrollbars obviously).

Now, this is isn't a big deal, and can be worked around by inserting CSS like this .navbar-collapse.collapsing.in { overflow: hidden; }, but if ui-bootstrap is supposed to just work out of the box given the official Bootstrap CSS, I think we should emulate the behaviour of Bootstrap JS as closely as possible. This PR tries to do just that by adding the in class in expandDone and removing it at the start of the collapsing transition. If you would prefer a different solution just let me know!

@inukshuk
Fix collapse animation
d307a1a 
Set class 'in' at the end of the animation to mimick the
behaviour of TWBS js. This removes scrollbars which are
otherwise visible during the animation in certain
configurations (most notably when used inside navbars).
@inukshuk
Copy link
Contributor Author

(Will look at the failing tests if you agree that we should indeed adjust the behaviour)

@Foxandxss
Copy link
Contributor

If it is for the better and it doesn't hurt anybody, I am OK with it.

@wesleycho
Copy link
Contributor

This is a quite interesting find, but I'm not sure these changes fixes this either - see this fork based on your changes here

@inukshuk
Copy link
Contributor Author

@wesleycho if you change the name of the directive to uib-collapse it works, though, or am I missing something?

@inukshuk
Copy link
Contributor Author

I updated the plunker here with the correct directive name -- there you should see that the scrollbar is not visible.

@wesleycho wesleycho added this to the 0.14.0 (Bootstrap 3.3) milestone Sep 30, 2015
@wesleycho
Copy link
Contributor

Ah, that is correct, I forgot about that!

Can you look into fixing the tests? This is a good fix.

@inukshuk
Copy link
Contributor Author

inukshuk commented Oct 1, 2015

@wesleycho thanks! I looked at the tests and noticed two separate issues. The big one is that $animate.flush does not seem to work as expected when $animateCss is used; in fact, when using $animateCss the tests only seem to work by accident right now. The problem is that the callbacks expandDone collapseDone are not called after $animate.flush() when the animation is set up using $animateCss -- if I don't use $animateCss then all is fine, the callbacks are called after flush and the tests are green.

Are you aware of this issue? I have not dug into $animateCss so just asking to see if you have any pointers for me. Thanks!

@icfantv
Copy link
Contributor

icfantv commented Oct 1, 2015

IIRC, angular 1.4.5 introduced an issue with $animate and we had to change all our tests to accommodate. It would not surprise me if $animate.flush() didn't work as expected.

@wesleycho wesleycho mentioned this pull request Oct 3, 2015
Closed
@wesleycho
Copy link
Contributor

The test failure is an angular bug - $animate in ngMock needs to flush all $animateCss animations as well.

@wesleycho wesleycho closed this in 6d1cd0f Oct 19, 2015
@bnjasim
Copy link

bnjasim commented Oct 20, 2015

Could you please look into this issue: http://stackoverflow.com/questions/33225665/why-does-finally-and-done-of-animatecss-behave-differently-in-ui-bootstrap-angu

@Foxandxss
Copy link
Contributor

I went to change it to finally because with .done it was never triggering a $digest so the collapse wasnt working properly without ngAnimate. With finally it works correctly both without ngAnimate and with ngAnimate. There was a bug with $animateCss that got resolved with 1.4.5.

What you see in our demos is using done and not finally, the finally is still on master and not in the demos.

So, is there any problem or there is something wrong? I would like to fix it if my solution wasn't the best one.

@Foxandxss
Copy link
Contributor

I have to admit that animation in angular is a bit weird so it wouldnt be weird if I missed something important here.

@bnjasim
Copy link

bnjasim commented Oct 20, 2015

Hey, Thanks for the prompt reply. I was implementing a sliding panel from left (for mobile displays) tweaking the collapse code. I guess I will go with basic css3 animation for the time being.

@icfantv
Copy link
Contributor

icfantv commented Oct 21, 2015

@bnjasim please do not use the comment section on PRs (or the issues forum) for support related requests. Rather, please follow the instructions here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
component: collapse
Projects
None yet
Milestone
0.14.3
Development

Successfully merging this pull request may close these issues.
5 participants
@inukshuk @Foxandxss @wesleycho @icfantv @bnjasim